Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gcc 10 compatibility for Drop alt #9485

Merged
merged 2 commits into from
Dec 6, 2020

Conversation

DelusionalLogic
Copy link

According to the official documentation[1] gcc 10 is more strict about
correct extern usage.

I've had to split the definition and declaration of dmac_desc and dmac_desc_wb
from i2c_master.{c,h}. This could be an issue if anyone includes the
i2c_master.h file without liking with the object file.

I've also had to remove the keymap_config definition from all the massdrop alt
keymaps, since it's defined in magic.c as well. The same would have to be done
for other keymaps. It doesn't seem like any of the in-tree keymaps use the
variable anyway. Anyone using it would have to declare it extern in their
keymap.

With these changes all the in-tree keymaps for the massdrop alt now compiles in
gcc 10

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

Jesper Jensen added 2 commits June 20, 2020 12:24
According to the official documentation[1] gcc 10 is more strict about
correct extern usage.

I've had to move the definition of dmac_desc and dmac_desc_wb from
i2c_master.h to the corresponding .c file. This could be an issue if
anyone includes the i2c_master.h file without liking with the object
file.

[1]: https://gcc.gnu.org/gcc-10/porting_to.html
The keymap_config def was conflicting with the one found in
tmk_core/common/magic.c. Declaring it extern in magic.c breaks a bunch
of keyboard that rely on that declaration (like the ergodox). Instead
I've removed the one found in the keymap.c of the massdrop alt.

The same change will have to be made to other keyboards.
@zvecr zvecr added the breaking_change Changes that need to wait for a version increment label Jun 20, 2020
@DelusionalLogic
Copy link
Author

For good measure I should probably say that compiling with -fcommon is also an option.

@DelusionalLogic
Copy link
Author

@zvecr Could i ask what the breaking change here is? Keymaps that don't remove keymap_config won't compile with gcc 10 (as they won't right now), but will still continue to be compatible with previous gcc versions.

That is to say, people that don't want to remove keymap_config won't be any more broken after this patch.

@zvecr
Copy link
Member

zvecr commented Jun 20, 2020

From the contributing guide which you have ticked as read,

IMPORTANT: If you would like to contribute a bugfix or improvement to user code, such as non-default keymaps, userspace and layouts, be sure to tag the original submitter of the code in your PR. Many users, regardless of skill level with Git and GitHub, may be confused or frustrated at their code being modified without their knowledge.

And also https://docs.qmk.fm/#/breaking_changes_instructions,

Edits to User Keymaps A user may submit their keymap to QMK, then some time later open a pull request with further updates, only to find it can’t be merged because it was edited in the qmk/qmk_firmware repository. As not all users are proficient at using Git or GitHub, the user may find themself unable to fix the issue on their own.

@DelusionalLogic
Copy link
Author

DelusionalLogic commented Jun 20, 2020

Fair enough. I figured the since the change didn't really change anything that mattered it was ok, but I see the merge angle being a problem for non-dev users.

Would it be better if I split the changes into an i2c part and a keymap part? that way the i2c part wont be breaking.

Another option would be to forego changing any keymaps except default and let the users change them themselves.

Do you have a preference for either, or should I just leave it as is?

@drashna
Copy link
Member

drashna commented Jul 5, 2020

Fair enough. I figured the since the change didn't really change anything that mattered it was ok, but I see the merge angle being a problem for non-dev users.

That may be, but issues arise when users want to update their repo. Not everyone is savvy with git, and handling merge conflicts. We want to reduce that, or at least reduce the frequency of it happening.

And I think the best option would be to target develop, and add the breaking change info.

@FabSchwul
Copy link

Thanks to this PR I could compile my Drop CTRL

@fauxpark
Copy link
Member

If we get signoff from @abishalom, @bontakun, @favorable-mutation and @reywood, this can go straight into master.

@favorable-mutation
Copy link
Contributor

LGTM! Thanks for adding this fix!

@reywood
Copy link
Contributor

reywood commented Oct 19, 2020

👍 looks good

@drashna
Copy link
Member

drashna commented Oct 23, 2020

@abishalom @bontakun any objections?

@abishalom
Copy link
Contributor

Fine by me

@tzarc
Copy link
Member

tzarc commented Nov 10, 2020

Looks like it'll need to go to develop as @bontakun is non-responsive. Will give it another week and then point it at develop instead.

@tzarc tzarc changed the base branch from master to develop December 6, 2020 06:24
@tzarc tzarc merged commit 63d0665 into qmk:develop Dec 6, 2020
vmalloc added a commit to vmalloc/qmk_firmware that referenced this pull request Nov 22, 2021
BorisTestov pushed a commit to BorisTestov/qmk_firmware that referenced this pull request May 23, 2024
* Split dmac_desc declaration and definition

According to the official documentation[1] gcc 10 is more strict about
correct extern usage.

I've had to move the definition of dmac_desc and dmac_desc_wb from
i2c_master.h to the corresponding .c file. This could be an issue if
anyone includes the i2c_master.h file without liking with the object
file.

[1]: https://gcc.gnu.org/gcc-10/porting_to.html

* Remove the keymap_config definition from keymaps

The keymap_config def was conflicting with the one found in
tmk_core/common/magic.c. Declaring it extern in magic.c breaks a bunch
of keyboard that rely on that declaration (like the ergodox). Instead
I've removed the one found in the keymap.c of the massdrop alt.

The same change will have to be made to other keyboards.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking_change Changes that need to wait for a version increment core keymap
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants